Skip to content

async: remove inter-task wakeup stream from waitable set before cancelling#1638

Open
GodlyDonuts wants to merge 1 commit into
bytecodealliance:mainfrom
GodlyDonuts:fix-sync-cancel-inter-task-stream-in-set
Open

async: remove inter-task wakeup stream from waitable set before cancelling#1638
GodlyDonuts wants to merge 1 commit into
bytecodealliance:mainfrom
GodlyDonuts:fix-sync-cancel-inter-task-stream-in-set

Conversation

@GodlyDonuts

Copy link
Copy Markdown

cancel_inter_task_stream_read issues a synchronous stream.cancel-read on the
inter-task wakeup stream while that stream is still a member of the task's
waitable set, calling remove_waitable only afterwards:

unsafe {
    UnitStreamOps.cancel_read(handle);   // sync cancel — stream still in the set
}
self.remove_waitable(handle);            // removed afterwards

Per component-model#647, a synchronous {stream,future}.cancel-{read,write}
must trap if the waitable is still in a waitable set, for the same reason the
synchronous read/write operations do (the cancel may need to block on the very
waitable the set is watching). The canonical ABI codifies this as
trap_if(e.in_waitable_set() and not async_).

The general cancellation path in WaitableOperation::cancel already respects
this — it calls unregister_waker (removing the waitable from the set) before
invoking the synchronous cancel intrinsic. cancel_inter_task_stream_read is
the one place that doesn't follow that ordering, so on a runtime that enforces
the trap it faults during ordinary async operation.

This reorders the two operations so the stream leaves the waitable set before
the synchronous cancel, matching the established unregister-then-cancel pattern.
The cancel's return code is discarded exactly as before, so behavior is
otherwise unchanged.

Context / testing

This surfaced while implementing the corresponding cancel traps in Wasmtime
(bytecodealliance/wasmtime#13690): once Wasmtime traps on a synchronous cancel
of an in-set waitable, every wasi:http@0.3.0 (p3) program built with this
runtime traps in cancel_inter_task_stream_read. With this change applied
(via a local [patch.crates-io]), Wasmtime's full p3 HTTP/CLI suites pass
again (52 previously-failing tests across wasmtime-wasi/wasmtime-wasi-http).

…ancelling

`cancel_inter_task_stream_read` issued a synchronous `stream.cancel-read` on
the inter-task wakeup stream while that stream was still a member of the task's
waitable set, only calling `remove_waitable` afterwards.

Per the Component Model (component-model#647), a synchronous
`{stream,future}.cancel-{read,write}` traps if the waitable is still in a
waitable set, for the same reason synchronous reads/writes do. Runtimes that
enforce this trap (e.g. recent Wasmtime) therefore fault here during ordinary
async operation.

Reorder so the stream leaves the waitable set before the synchronous cancel,
matching the unregister-then-cancel ordering already used by the general
`WaitableOperation::cancel` path. The cancel result is discarded as before, so
behavior is otherwise unchanged.
@GodlyDonuts

Copy link
Copy Markdown
Author

Companion to bytecodealliance/wasmtime#13708, which adds the matching "in a waitable set" trap for synchronous {stream,future}.cancel-{read,write} and subtask.cancel (per component-model#647). Enforcing that trap in Wasmtime is what surfaces this; with this PR patched in locally, Wasmtime's full p3 suites pass again. cc @dicej

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant